Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto-scale vtgate with HPA #598

Merged
merged 25 commits into from
Oct 14, 2024
Merged

Conversation

siadat
Copy link
Contributor

@siadat siadat commented Aug 30, 2024

Problem

We'd like to be able to auto-scale number of vtgate replicas using HorizontalPodAutoscaler.

Solution

In order to make a CRD scalable by HPA, it should define the Scale subresource.

We also make sure if a vtgate is being autoscaled, the replicas value is not copied from VitessCluster. Otherwise, autoscaled values will be overwritten.

Context

https://vitess.slack.com/archives/CNE9WP677/p1725015568336609

@siadat siadat changed the title Make VitessCell scalable by HPA Scale vtgate with HPA Sep 10, 2024
@siadat siadat changed the title Scale vtgate with HPA Auto-scale vtgate with HPA Sep 10, 2024
Copy link
Member

@frouioui frouioui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution 🚀 Would it be possible to add an end-to-end test for this? We have examples of other features being covered by E2E tests under ./test/endtoend/.

pkg/operator/vtgate/horizontal_pod_autoscaler.go Outdated Show resolved Hide resolved
Comment on lines +65 to +67
autoscaler:
properties:
behavior:
properties:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ./test/endtoend/operator/operator-latest.yaml file we use for our tests must be updated with the newly compiled CRDs.

To do this, you can use the following command: kustomize build ./deploy > build/_output/operator.yaml. The output found in ./build/_output/operator.yamlcan be copied over the test file.

The changes made to the Deployment named vitess-operator will have to be reverted before pushing, as the new file you'll copy will override some defaults we use for our tests.

Finally, the changes made to ./test/endtoend/operator/operator-latest.yaml will have to be copied to the Vitess repository in a separate PR, into the file ./examples/operator/operator.yaml.

pkg/operator/vtgate/horizontal_pod_autoscaler.go Outdated Show resolved Hide resolved
pkg/controller/vitesscell/reconcile_vtgate.go Show resolved Hide resolved
Copy link
Member

@frouioui frouioui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me so far, outside of the few comments I left.

Comment on lines 11 to 15
vtctld: vitess/lite:v20.0.1
vtgate: vitess/lite:v20.0.1
vttablet: vitess/lite:v20.0.1
vtorc: vitess/lite:v20.0.1
vtbackup: vitess/lite:v20.0.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tags should be latest. On main we want to test the latest tag of Vitess (main branch) with the PR's version of the vitess-operator - expect when running the upgrade test (which first uses the latest release of Vitess, and then upgrades to the latest tag of Vitess).

deploy/crds/planetscale.com_vitesscells.yaml Show resolved Hide resolved
Makefile Show resolved Hide resolved
@siadat siadat force-pushed the sina/vitesscell-hpa branch 3 times, most recently from be080bc to cc8b9c1 Compare September 30, 2024 18:16
@siadat siadat force-pushed the sina/vitesscell-hpa branch 12 times, most recently from 4d7f153 to a2dc6ac Compare October 4, 2024 01:08
@siadat
Copy link
Contributor Author

siadat commented Oct 4, 2024

Thanks for the reviews! I don't have access to see the buildkite logs to debug it.

Signed-off-by: Sina Siadat <[email protected]>
Signed-off-by: Sina Siadat <[email protected]>
Signed-off-by: Sina Siadat <[email protected]>
It was using the name of the vtgate deployment, while it should be using
the name of the VitessCell.

Signed-off-by: Sina Siadat <[email protected]>
Signed-off-by: Sina Siadat <[email protected]>
@siadat siadat force-pushed the sina/vitesscell-hpa branch 2 times, most recently from 919c890 to 07efe52 Compare October 9, 2024 00:24
// Behavior specifies the scaling behavior of the target in both Up and Down directions.
// +optional
Behavior *autoscalingv2.HorizontalPodAutoscalerBehavior `json:"behavior,omitempty"`
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frouioui Let me know if I need to make any changes to this spec. It is similar to HorizontalPodAutoscaler, without the ScaleTargetRef field, whose value I am setting in https://github.com/siadat/vitess-operator/blob/sina/vitesscell-hpa/pkg/operator/vtgate/horizontal_pod_autoscaler.go#L46-L50

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine. If you don't want the end-user to customize ScaleTargetRef, which makes sense as it should be constant, then having our own AutoscalerSpec is okay.

Copy link
Member

@frouioui frouioui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for this valuable contribution.

Let's make sure the operator.yaml file is updated on the Vitess PR (vitessio/vitess#16805)

Copy link
Collaborator

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thank you!

@frouioui frouioui merged commit 41a5e3f into planetscale:main Oct 14, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants